Handle constant B-spline mutual information inputs - #9022
Conversation
Signed-off-by: kyinhub <kevinpyin@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe B-spline kernel now requires more than four bins. Bin-size computation handles constant-intensity inputs without division by zero, and half-precision inputs use float computations for range and bin calculations. Tests cover invalid bin counts, constant inputs, extreme ranges, finite losses, and finite gradients. Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monai/losses/image_dissimilarity.py`:
- Around line 233-240: Update the initialization logic around the kernel_type
validation and sigma calculation to reject num_bins=1 for Gaussian kernels
before computing bin-centre spacing, while preserving the existing b-spline
constraint. Add regression coverage confirming that a one-bin Gaussian
configuration raises the expected validation error.
- Around line 293-295: Update the range and bin-size arithmetic in the image
dissimilarity loss to compute intermediate values in float32, apply the
zero-range fallback there, then cast the resulting bin size back to the image
gradient dtype. In
tests/losses/image_dissimilarity/test_global_mutual_information_loss.py, add
small- and large-nonzero-range float16 cases asserting finite loss and
gradients.
In `@tests/losses/image_dissimilarity/test_global_mutual_information_loss.py`:
- Around line 175-179: Extend the constant-image coverage in
test_b_spline_constant_images_are_finite with separate cases for a constant
prediction against a varying target and a varying prediction against a constant
target. Keep the existing finite-loss and gradient assertions, and ensure both
independent zero-intensity-range paths are exercised by the tests.
- Around line 189-193: Update the float16 B-spline tests, including
test_b_spline_constant_half_precision_images_are_finite, to use an
overflow-triggering bin configuration such as num_bins=64 instead of 32.
Preserve the existing assertions and test behavior while ensuring the cubic term
can exceed the float16 range.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f51b91f4-41e6-4767-9ba0-c9a9ebbf32a0
📒 Files selected for processing (2)
monai/losses/image_dissimilarity.pytests/losses/image_dissimilarity/test_global_mutual_information_loss.py
Signed-off-by: kyinhub <kevinpyin@gmail.com>
|
CI triage: the only failed check is |
|
The only failing check is the macOS full-dep job (89732744347), in the unrelated distributed PersistentDataset test: Gloo cannot resolve the runner .local hostname ( |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/losses/image_dissimilarity/test_global_mutual_information_loss.py (1)
175-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the parameterized test arguments.
Add Google-style
Args:sections forconstant_input,dtype, andmaximum; document_as the parameterized case label or rename it tocase_name.As per path instructions, docstrings should describe each variable, return value, and raised exception in Google-style sections.
Also applies to: 216-217
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/losses/image_dissimilarity/test_global_mutual_information_loss.py` around lines 175 - 177, Update the docstrings for the parameterized tests, including test_b_spline_single_constant_input_is_finite and the additionally referenced test, with Google-style Args sections documenting constant_input, dtype, and maximum. Document the parameterized case-label argument as _ or rename it to case_name, and describe each argument’s purpose without changing test behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/losses/image_dissimilarity/test_global_mutual_information_loss.py`:
- Around line 175-177: Update the docstrings for the parameterized tests,
including test_b_spline_single_constant_input_is_finite and the additionally
referenced test, with Google-style Args sections documenting constant_input,
dtype, and maximum. Document the parameterized case-label argument as _ or
rename it to case_name, and describe each argument’s purpose without changing
test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ea93d41-c1d2-4149-9e0d-16831277d3ca
📒 Files selected for processing (2)
monai/losses/image_dissimilarity.pytests/losses/image_dissimilarity/test_global_mutual_information_loss.py
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/losses/image_dissimilarity.py
Signed-off-by: kyinhub <kevinpyin@gmail.com>
|
Addressed the latest CodeRabbit documentation nit in |
Signed-off-by: kyinhub <kevinpyin@gmail.com>
Signed-off-by: kyinhub <kevinpyin@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/losses/image_dissimilarity.py (1)
294-303: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid calling
torch.finfoon integer inputs.
compute_imgkeeps integer dtypes unchanged, sotorch.finfo(compute_img.dtype).tinycrashes before the fallback. Promote non-floating inputs tofloat32, or reject them explicitly, and add a mixed floating-prediction/integer-target regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monai/losses/image_dissimilarity.py` around lines 294 - 303, Update the dtype handling around compute_img so integer inputs are promoted to float32 before calling torch.finfo and computing bin-size validity, while preserving the existing float16/bfloat16 promotion behavior. Add a regression test covering a floating-point prediction with an integer target and verify the loss completes without the torch.finfo dtype error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@monai/losses/image_dissimilarity.py`:
- Around line 294-303: Update the dtype handling around compute_img so integer
inputs are promoted to float32 before calling torch.finfo and computing bin-size
validity, while preserving the existing float16/bfloat16 promotion behavior. Add
a regression test covering a floating-point prediction with an integer target
and verify the loss completes without the torch.finfo dtype error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ff6ca5bc-a916-49bb-bd62-e36268ab5e22
📒 Files selected for processing (2)
monai/losses/image_dissimilarity.pytests/losses/image_dissimilarity/test_global_mutual_information_loss.py
|
Addressed the latest integer-dtype review finding in |
Signed-off-by: kyinhub <kevinpyin@gmail.com>
2e3e29d to
354e597
Compare
1 participant
Fixes #9018.
Description
B-spline global mutual information now remains finite when either prediction or target has a constant intensity range and for float16, bfloat16, or float32 inputs with tiny nonzero ranges. B-spline configurations with four or fewer bins are rejected because the implementation reserves two padding bins at each boundary. Gaussian configurations with one or fewer bins are rejected before bin-centre spacing is computed.
The fix performs reduced-precision range, bin-width, and B-spline coordinate arithmetic in float32. Bin widths too small to be represented safely by the compute or input dtype are treated as a degenerate range. This prevents zero-range division, tiny-range underflow, unrepresentable normalization slopes, and cubic-distance overflow while preserving autograd back to the original input.
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.Validation
num_bins=64;354e5975affected loss module: 24 passed, 1 skipped;git diff --checkpass.